Skip to content

update refresh-token-grant errors #140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 11 commits into from

Conversation

chrisfranko
Copy link
Contributor

@chrisfranko chrisfranko commented Apr 13, 2022

Two different conditions shouldnt throw the same error

Summary

The main problem is various errors were identical in the Refresh Token Grant file. While working on an express/mongodb adapter I kept erroring out at the client check. Well the client check error is the same error that exist in like 5 other places. Just changing the verbiage alone would help in diagnosing the problem.

Linked issue(s)

Involved parts of the project

[https://github.com/node-oauth/node-oauth2-server/blob/master/lib/grant-types/refresh-token-grant-type.js](Refresh Grant Type)

Added tests?

si

OAuth2 standard

Its just the error verbiage the logic remains untouched.

Reproduction

No new features just changing some text.

dependabot bot and others added 7 commits February 1, 2022 18:18
Bumps [sinon](https://github.com/sinonjs/sinon) from 12.0.1 to 13.0.1.
- [Release notes](https://github.com/sinonjs/sinon/releases)
- [Changelog](https://github.com/sinonjs/sinon/blob/master/docs/changelog.md)
- [Commits](sinonjs/sinon@v12.0.1...v13.0.1)

---
updated-dependencies:
- dependency-name: sinon
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [mocha](https://github.com/mochajs/mocha) from 9.1.2 to 9.2.2.
- [Release notes](https://github.com/mochajs/mocha/releases)
- [Changelog](https://github.com/mochajs/mocha/blob/master/CHANGELOG.md)
- [Commits](mochajs/mocha@v9.1.2...v9.2.2)

---
updated-dependencies:
- dependency-name: mocha
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [eslint](https://github.com/eslint/eslint) from 8.2.0 to 8.11.0.
- [Release notes](https://github.com/eslint/eslint/releases)
- [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md)
- [Commits](eslint/eslint@v8.2.0...v8.11.0)

---
updated-dependencies:
- dependency-name: eslint
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
…yarn/eslint-8.11.0

build(deps-dev): bump eslint from 8.2.0 to 8.11.0
…yarn/mocha-9.2.2

build(deps-dev): bump mocha from 9.1.2 to 9.2.2
…yarn/sinon-13.0.1

build(deps-dev): bump sinon from 12.0.1 to 13.0.1
Two different conditions shouldnt throw the same error
@jankapunkt jankapunkt changed the base branch from master to development April 14, 2022 11:32
@jankapunkt
Copy link
Member

@chrisfranko thank you for contributing to this project. Please complete the sections from the issue template, resolve the merge confliczts and update the tests accordingly. We use development as our main PR target (more see here)

Since this would be a breaking change we first need to discuss this with respect to the RFC. @Uzlopak @HappyZombies @jorenvandeweyer

Copy link
Collaborator

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change them like I remarked.

Also unit tests should be adapted.

chrisfranko and others added 2 commits April 14, 2022 08:01
Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>
Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>
@HappyZombies
Copy link
Member

@jankapunkt how could this be a breaking change? It's just changing the error message.

Though someone could be testing for this particular error message, but eh, that would be wrong of them to test.

@jankapunkt
Copy link
Member

@HappyZombies sorry I misread it. It's only the error message that changed while I thought it's the error class that changed. From that point it should be a good improvement. The package.json and package-lock.json should be restored to the previous values and the yarn.lock should be removed then it's good to go imo.

@chrisfranko
Copy link
Contributor Author

Done!

@HappyZombies
Copy link
Member

@chrisfranko dope thanks, but can we remove the sinon update and remove the lock file changes. Then we will approve and merge

@chrisfranko
Copy link
Contributor Author

Yea man I thought I did that?

@chrisfranko
Copy link
Contributor Author

55e236c

But honestly im not entirely sure how to remove the sinon update because those all came from master and I didnt manually do them. However! It might just be easier for me to push a new pr to the dev branch instead.

@chrisfranko
Copy link
Contributor Author

#143

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants